-
Notifications
You must be signed in to change notification settings - Fork 14.6k
[TSan][compiler-rt] Move symbolization in ReportRace outside of locks on Apple platforms #149516
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[TSan][compiler-rt] Move symbolization in ReportRace outside of locks on Apple platforms #149516
Conversation
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Dan Blackwell (DanBlackwell) ChangesOther platforms should still symbolize within the locked region. rdar://152829396 Full diff: https://github.com/llvm/llvm-project/pull/149516.diff 9 Files Affected:
diff --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
index 14b25a8995dab..274a38390bda8 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
@@ -2146,6 +2146,7 @@ static void ReportErrnoSpoiling(ThreadState *thr, uptr pc, int sig) {
rep.SetSigNum(sig);
if (!IsFiredSuppression(ctx, ReportTypeErrnoInSignal, stack)) {
rep.AddStack(stack, true);
+ rep.SymbolizeStackElems();
OutputReport(thr, rep);
}
}
diff --git a/compiler-rt/lib/tsan/rtl/tsan_interface_ann.cpp b/compiler-rt/lib/tsan/rtl/tsan_interface_ann.cpp
index befd6a369026d..d16fd4cfb00d9 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_interface_ann.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_interface_ann.cpp
@@ -446,6 +446,7 @@ static void ReportMutexHeldWrongContext(ThreadState *thr, uptr pc) {
VarSizeStackTrace trace;
ObtainCurrentStack(thr, pc, &trace);
rep.AddStack(trace, true);
+ rep.SymbolizeStackElems();
OutputReport(thr, rep);
}
diff --git a/compiler-rt/lib/tsan/rtl/tsan_mman.cpp b/compiler-rt/lib/tsan/rtl/tsan_mman.cpp
index 0ea83fb3b5982..3a0a6a1a4564f 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_mman.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_mman.cpp
@@ -185,6 +185,7 @@ static void SignalUnsafeCall(ThreadState *thr, uptr pc) {
ThreadRegistryLock l(&ctx->thread_registry);
ScopedReport rep(ReportTypeSignalUnsafe);
rep.AddStack(stack, true);
+ rep.SymbolizeStackElems();
OutputReport(thr, rep);
}
diff --git a/compiler-rt/lib/tsan/rtl/tsan_report.h b/compiler-rt/lib/tsan/rtl/tsan_report.h
index bfe470797f8f7..50501060840a4 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_report.h
+++ b/compiler-rt/lib/tsan/rtl/tsan_report.h
@@ -12,6 +12,8 @@
#ifndef TSAN_REPORT_H
#define TSAN_REPORT_H
+#include "sanitizer_common/sanitizer_internal_defs.h"
+#include "sanitizer_common/sanitizer_stacktrace.h"
#include "sanitizer_common/sanitizer_symbolizer.h"
#include "sanitizer_common/sanitizer_thread_registry.h"
#include "sanitizer_common/sanitizer_vector.h"
@@ -56,6 +58,7 @@ struct ReportMop {
bool atomic;
uptr external_tag;
Vector<ReportMopMutex> mset;
+ StackTrace stack_trace;
ReportStack *stack;
ReportMop();
@@ -79,6 +82,7 @@ struct ReportLocation {
int fd = 0;
bool fd_closed = false;
bool suppressable = false;
+ StackID stack_id = 0;
ReportStack *stack = nullptr;
};
@@ -89,12 +93,15 @@ struct ReportThread {
ThreadType thread_type;
char *name;
Tid parent_tid;
+ StackID stack_id;
ReportStack *stack;
+ bool suppressable;
};
struct ReportMutex {
int id;
uptr addr;
+ StackID stack_id;
ReportStack *stack;
};
@@ -105,6 +112,7 @@ class ReportDesc {
Vector<ReportStack*> stacks;
Vector<ReportMop*> mops;
Vector<ReportLocation*> locs;
+ Vector<uptr> added_location_addrs;
Vector<ReportMutex*> mutexes;
Vector<ReportThread*> threads;
Vector<Tid> unique_tids;
diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl.h b/compiler-rt/lib/tsan/rtl/tsan_rtl.h
index dc32980e905f2..7122c15bb2c13 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_rtl.h
+++ b/compiler-rt/lib/tsan/rtl/tsan_rtl.h
@@ -420,6 +420,7 @@ class ScopedReportBase {
void AddSleep(StackID stack_id);
void SetCount(int count);
void SetSigNum(int sig);
+ void SymbolizeStackElems(void);
const ReportDesc *GetReport() const;
diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp
index 487fa490636eb..0d133aead1ee1 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp
@@ -419,6 +419,10 @@ NOINLINE void TraceRestartMemoryAccess(ThreadState* thr, uptr pc, uptr addr,
ALWAYS_INLINE USED void MemoryAccess(ThreadState* thr, uptr pc, uptr addr,
uptr size, AccessType typ) {
+#if SANITIZER_APPLE
+ if (thr->in_symbolizer)
+ return;
+#endif
RawShadow* shadow_mem = MemToShadow(addr);
UNUSED char memBuf[4][64];
DPrintf2("#%d: Access: %d@%d %p/%zd typ=0x%x {%s, %s, %s, %s}\n", thr->tid,
diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp
index 2a8aa1915c9ae..f546931ff0e56 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp
@@ -62,6 +62,7 @@ static void ReportMutexMisuse(ThreadState *thr, uptr pc, ReportType typ,
ObtainCurrentStack(thr, pc, &trace);
rep.AddStack(trace, true);
rep.AddLocation(addr, 1);
+ rep.SymbolizeStackElems();
OutputReport(thr, rep);
}
@@ -539,13 +540,16 @@ void ReportDeadlock(ThreadState *thr, uptr pc, DDReport *r) {
for (int i = 0; i < r->n; i++) {
for (int j = 0; j < (flags()->second_deadlock_stack ? 2 : 1); j++) {
u32 stk = r->loop[i].stk[j];
+ StackTrace stack;
if (stk && stk != kInvalidStackID) {
- rep.AddStack(StackDepotGet(stk), true);
+ stack = StackDepotGet(stk);
} else {
// Sometimes we fail to extract the stack trace (FIXME: investigate),
// but we should still produce some stack trace in the report.
- rep.AddStack(StackTrace(&dummy_pc, 1), true);
+ stack = StackTrace(&dummy_pc, 1);
}
+ rep.AddStack(stack, true);
+ rep.SymbolizeStackElems();
}
}
OutputReport(thr, rep);
@@ -572,6 +576,7 @@ void ReportDestroyLocked(ThreadState *thr, uptr pc, uptr addr,
return;
rep.AddStack(trace, true);
rep.AddLocation(addr, 1);
+ rep.SymbolizeStackElems();
OutputReport(thr, rep);
}
diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp
index 0820bf1adee43..991e5c60399f0 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp
@@ -10,6 +10,8 @@
//
//===----------------------------------------------------------------------===//
+#include <alloca.h>
+
#include "sanitizer_common/sanitizer_common.h"
#include "sanitizer_common/sanitizer_libc.h"
#include "sanitizer_common/sanitizer_placement_new.h"
@@ -187,10 +189,8 @@ void ScopedReportBase::AddMemoryAccess(uptr addr, uptr external_tag, Shadow s,
mop->size = size;
mop->write = !(typ & kAccessRead);
mop->atomic = typ & kAccessAtomic;
- mop->stack = SymbolizeStack(stack);
mop->external_tag = external_tag;
- if (mop->stack)
- mop->stack->suppressable = true;
+ mop->stack_trace = stack;
for (uptr i = 0; i < mset->Size(); i++) {
MutexSet::Desc d = mset->Get(i);
int id = this->AddMutex(d.addr, d.stack_id);
@@ -199,6 +199,45 @@ void ScopedReportBase::AddMemoryAccess(uptr addr, uptr external_tag, Shadow s,
}
}
+void ScopedReportBase::SymbolizeStackElems() {
+ // symbolize memory ops
+ for (usize i = 0; i < rep_->mops.Size(); i++) {
+ ReportMop *mop = rep_->mops[i];
+ mop->stack = SymbolizeStack(mop->stack_trace);
+ if (mop->stack)
+ mop->stack->suppressable = true;
+ }
+
+ // symbolize locations
+ for (usize i = 0; i < rep_->locs.Size(); i++) {
+ ReportLocation *loc = rep_->locs[i];
+ loc->stack = SymbolizeStackId(loc->stack_id);
+ }
+
+ for (usize i = 0; i < rep_->added_location_addrs.Size(); i++) {
+ if (ReportLocation *loc = SymbolizeData(rep_->added_location_addrs[i])) {
+ loc->suppressable = true;
+ // TODO: These all end up at the back of locs - line them up with their
+ // corresponding location
+ rep_->locs.PushBack(loc);
+ }
+ }
+
+ // symbolize threads
+ for (usize i = 0; i < rep_->threads.Size(); i++) {
+ ReportThread *rt = rep_->threads[i];
+ rt->stack = SymbolizeStackId(rt->stack_id);
+ if (rt->stack)
+ rt->stack->suppressable = rt->suppressable;
+ }
+
+ // symbolize mutexes
+ for (usize i = 0; i < rep_->mutexes.Size(); i++) {
+ ReportMutex *rm = rep_->mutexes[i];
+ rm->stack = SymbolizeStackId(rm->stack_id);
+ }
+}
+
void ScopedReportBase::AddUniqueTid(Tid unique_tid) {
rep_->unique_tids.PushBack(unique_tid);
}
@@ -216,10 +255,8 @@ void ScopedReportBase::AddThread(const ThreadContext *tctx, bool suppressable) {
rt->name = internal_strdup(tctx->name);
rt->parent_tid = tctx->parent_tid;
rt->thread_type = tctx->thread_type;
- rt->stack = 0;
- rt->stack = SymbolizeStackId(tctx->creation_stack_id);
- if (rt->stack)
- rt->stack->suppressable = suppressable;
+ rt->stack_id = tctx->creation_stack_id;
+ rt->suppressable = suppressable;
}
#if !SANITIZER_GO
@@ -270,7 +307,7 @@ int ScopedReportBase::AddMutex(uptr addr, StackID creation_stack_id) {
rep_->mutexes.PushBack(rm);
rm->id = rep_->mutexes.Size() - 1;
rm->addr = addr;
- rm->stack = SymbolizeStackId(creation_stack_id);
+ rm->stack_id = creation_stack_id;
return rm->id;
}
@@ -288,7 +325,7 @@ void ScopedReportBase::AddLocation(uptr addr, uptr size) {
loc->fd_closed = closed;
loc->fd = fd;
loc->tid = creat_tid;
- loc->stack = SymbolizeStackId(creat_stack);
+ loc->stack_id = creat_stack;
rep_->locs.PushBack(loc);
AddThread(creat_tid);
return;
@@ -310,7 +347,7 @@ void ScopedReportBase::AddLocation(uptr addr, uptr size) {
loc->heap_chunk_size = b->siz;
loc->external_tag = b->tag;
loc->tid = b->tid;
- loc->stack = SymbolizeStackId(b->stk);
+ loc->stack_id = b->stk;
rep_->locs.PushBack(loc);
AddThread(b->tid);
return;
@@ -324,11 +361,7 @@ void ScopedReportBase::AddLocation(uptr addr, uptr size) {
AddThread(tctx);
}
#endif
- if (ReportLocation *loc = SymbolizeData(addr)) {
- loc->suppressable = true;
- rep_->locs.PushBack(loc);
- return;
- }
+ rep_->added_location_addrs.PushBack(addr);
}
#if !SANITIZER_GO
@@ -761,65 +794,91 @@ void ReportRace(ThreadState *thr, RawShadow *shadow_mem, Shadow cur, Shadow old,
DynamicMutexSet mset1;
MutexSet *mset[kMop] = {&thr->mset, mset1};
- // We need to lock the slot during RestoreStack because it protects
- // the slot journal.
- Lock slot_lock(&ctx->slots[static_cast<uptr>(s[1].sid())].mtx);
- ThreadRegistryLock l0(&ctx->thread_registry);
- Lock slots_lock(&ctx->slot_mtx);
- if (SpuriousRace(old))
- return;
- if (!RestoreStack(EventType::kAccessExt, s[1].sid(), s[1].epoch(), addr1,
- size1, typ1, &tids[1], &traces[1], mset[1], &tags[1])) {
- StoreShadow(&ctx->last_spurious_race, old.raw());
- return;
- }
+#if SANITIZER_APPLE
+ static Mutex report_write_mutex;
+ Lock report_write_lock(&report_write_mutex);
+ ScopedReport *_rep = (ScopedReport *)alloca(sizeof(ScopedReport));
+#endif
+ // Take a new scope as Apple platforms need to release the below locks,
+ // before writing out the report, in order to avoid a deadlock
+ {
+ // We need to lock the slot during RestoreStack because it protects
+ // the slot journal.
+ Lock slot_lock(&ctx->slots[static_cast<uptr>(s[1].sid())].mtx);
+ ThreadRegistryLock l0(&ctx->thread_registry);
+ Lock slots_lock(&ctx->slot_mtx);
+ if (SpuriousRace(old))
+ return;
+ if (!RestoreStack(EventType::kAccessExt, s[1].sid(), s[1].epoch(), addr1,
+ size1, typ1, &tids[1], &traces[1], mset[1], &tags[1])) {
+ StoreShadow(&ctx->last_spurious_race, old.raw());
+ return;
+ }
- if (IsFiredSuppression(ctx, rep_typ, traces[1]))
- return;
+ if (IsFiredSuppression(ctx, rep_typ, traces[1]))
+ return;
- if (HandleRacyStacks(thr, traces))
- return;
+ if (HandleRacyStacks(thr, traces))
+ return;
- // If any of the accesses has a tag, treat this as an "external" race.
- uptr tag = kExternalTagNone;
- for (uptr i = 0; i < kMop; i++) {
- if (tags[i] != kExternalTagNone) {
- rep_typ = ReportTypeExternalRace;
- tag = tags[i];
- break;
+ // If any of the accesses has a tag, treat this as an "external" race.
+ uptr tag = kExternalTagNone;
+ for (uptr i = 0; i < kMop; i++) {
+ if (tags[i] != kExternalTagNone) {
+ rep_typ = ReportTypeExternalRace;
+ tag = tags[i];
+ break;
+ }
}
- }
- ScopedReport rep(rep_typ, tag);
- for (uptr i = 0; i < kMop; i++)
- rep.AddMemoryAccess(addr, tags[i], s[i], tids[i], traces[i], mset[i]);
+#if SANITIZER_APPLE
+ new (_rep) ScopedReport(rep_typ, tag);
+ ScopedReport &rep = *_rep;
+#else
+ ScopedReport rep(rep_typ, tag);
+#endif
+ for (uptr i = 0; i < kMop; i++)
+ rep.AddMemoryAccess(addr, tags[i], s[i], tids[i], traces[i], mset[i]);
- for (uptr i = 0; i < kMop; i++) {
- ThreadContext *tctx = static_cast<ThreadContext *>(
- ctx->thread_registry.GetThreadLocked(tids[i]));
- rep.AddThread(tctx);
- }
+ for (uptr i = 0; i < kMop; i++) {
+ ThreadContext *tctx = static_cast<ThreadContext *>(
+ ctx->thread_registry.GetThreadLocked(tids[i]));
+ rep.AddThread(tctx);
+ }
- rep.AddLocation(addr_min, addr_max - addr_min);
-
- if (flags()->print_full_thread_history) {
- const ReportDesc *rep_desc = rep.GetReport();
- for (uptr i = 0; i < rep_desc->threads.Size(); i++) {
- Tid parent_tid = rep_desc->threads[i]->parent_tid;
- if (parent_tid == kMainTid || parent_tid == kInvalidTid)
- continue;
- ThreadContext *parent_tctx = static_cast<ThreadContext *>(
- ctx->thread_registry.GetThreadLocked(parent_tid));
- rep.AddThread(parent_tctx);
+ rep.AddLocation(addr_min, addr_max - addr_min);
+
+ if (flags()->print_full_thread_history) {
+ const ReportDesc *rep_desc = rep.GetReport();
+ for (uptr i = 0; i < rep_desc->threads.Size(); i++) {
+ Tid parent_tid = rep_desc->threads[i]->parent_tid;
+ if (parent_tid == kMainTid || parent_tid == kInvalidTid)
+ continue;
+ ThreadContext *parent_tctx = static_cast<ThreadContext *>(
+ ctx->thread_registry.GetThreadLocked(parent_tid));
+ rep.AddThread(parent_tctx);
+ }
}
- }
#if !SANITIZER_GO
- if (!((typ0 | typ1) & kAccessFree) &&
- s[1].epoch() <= thr->last_sleep_clock.Get(s[1].sid()))
- rep.AddSleep(thr->last_sleep_stack_id);
+ if (!((typ0 | typ1) & kAccessFree) &&
+ s[1].epoch() <= thr->last_sleep_clock.Get(s[1].sid()))
+ rep.AddSleep(thr->last_sleep_stack_id);
+#endif
+
+#if SANITIZER_APPLE
+ } // Close this scope to release the locks
+
+ _rep->SymbolizeStackElems();
+ OutputReport(thr, *_rep);
+
+ // Need to manually destroy this because we used placement new to allocate
+ _rep->~ScopedReport();
+#else
+ rep.SymbolizeStackElems();
+ OutputReport(thr, rep);
+ }
#endif
- OutputReport(thr, rep);
}
void PrintCurrentStack(ThreadState *thr, uptr pc) {
diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp
index 8d29e25a6dd20..d7083e3dace7b 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp
@@ -96,6 +96,7 @@ void ThreadFinalize(ThreadState *thr) {
ScopedReport rep(ReportTypeThreadLeak);
rep.AddThread(leaks[i].tctx, true);
rep.SetCount(leaks[i].count);
+ rep.SymbolizeStackElems();
OutputReport(thr, rep);
}
#endif
|
… on Apple platforms
64fa7e5
to
6eea085
Compare
I believe that this change is a similar concept to #113661, but hopefully it is a little easier to follow the changes. I am essentially collecting up all of the information required for symbolization, then releasing the locks, and finally symbolizing and outputting the report. I have tried to guard these changes so that the functionality for non-Apple platforms is unchanged; and the changes should only affect |
We've reviewed this internally, but would like to get an external opinion too. |
From the patch title, I was expecting this to have no changes for non-Apple platforms, but IIUC this pull request can be viewed as two parts:
Could you please split this into two separate patches? It's much easier to reason about the changes when they are in separate NFC-ish and |
@thurstond I think that is a good idea; I have put up a PR for the refactoring elements here: #151120. I will close this PR. |
Other platforms should still symbolize within the locked region.
rdar://152829396